Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't panic when invalid paths are configured #394

Closed
wants to merge 2 commits into from
Closed

Conversation

akshayjshah
Copy link
Contributor

If the config struct includes invalid output paths, make sure that we don't try
to execute a nil function pointer.

Fixes #390.

If the config struct includes invalid output paths, make sure that we don't try
to execute a nil function pointer.

Fixes #390.
@akshayjshah
Copy link
Contributor Author

Coveralls appears to be hosed somehow; the Coveralls builds themselves are passing, but they're not calling back to GitHub properly to green-light this PR.

If code reviewers think this looks okay, I'm going to force-land (as long as Travis builds continue to pass).

@prashantv
Copy link
Collaborator

This change looks fine to fix the issue in #390, but it seemed a little odd to have this partial return on error.

On errors in Open, I think we should either:

  • Return a WriteSyncer with successfully opened files, and a non-nil close function.
  • Return nil for both WriteSyncer and close.

I don't think there's any advantages to returning a close without a WriteSyncer.

Take a look at #396 which goes the latter route (we still collect all errors during Open, but if there's an error, no WriteSyncer nor close is returned.

@prashantv prashantv deleted the ajs-panics branch March 30, 2017 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants